-
Notifications
You must be signed in to change notification settings - Fork 390
feat(fcm): Support apns.live_activity_token
field in FCM ApnsConfig
#2891
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
@@ -1725,6 +1725,21 @@ describe('Messaging', () => { | |||
}); | |||
}); | |||
|
|||
const invalidApnsLiveActivityTokens: any[] = [null, NaN, 0, 1, true, false] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we should add a test for valid payload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing. I added the tests with valid payloads for the live activity start | update | end
events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! Could you change the new property to lowerCamelCase
formatting?
Can you also run npm run api-extractor:local
? That would update the api docs and should fix the CI errors. Thanks!
/** | ||
* APN `live_activity_push_to_start_token` or `live_activity_push_token` to start or update live activities. | ||
*/ | ||
live_activity_token?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exported names should be in lowerCamelCase
formatting. Could you change this to liveActivityToken
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thanks for your review!
I am not quite sure where to map the property liveActivityToken
to live_activity_token
as required for the HTTP API? https://firebase.google.com/docs/cloud-messaging/ios/live-activity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also: Should the additional fields like dismissal-date
content-state
or attributes-type
also get included and mapped? Those could also be [customData: string]: any
typed
* feat(fcm): Support `live_activity_token` field on `ApnsConfig` * added validation * update documentation
live_activity_token
field onApnsConfig
Fixes #2875